Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop incorrect port #49

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Peleg
Copy link

@Peleg Peleg commented Apr 6, 2020

Rack::Request#url includes the port of the original request (despite
accurate scheme updates). This becomes an issue when LB terminates SSL:

Since we only need the request url from the env, this changeset manually
fetches the correct scheme (Rack accounts for FWD headers already), the
host (w/out port as opposed to #host_with_port), and the fullpath
(pathname + query params)

Another benefit of this changeset is that it no longer mutates the
original env object (assigning to new_env is by reference).

Caveat: im dropping @options[:protocol] bc x-fwd-proto should
handle this already.

`Rack::Request#url` includes the port of the original request (despite
accurate scheme updates). This becomes an issue when LB terminates SSL:

- client request is made to https://example.com:443,
- LB request is http://example.com:80
- build_api_url returns correct scheme but wrong port:
  https://example:80

Since we only need the request url from the env, this changeset manually
fetches the correct scheme (Rack accounts for FWD headers already), the
host (w/out port as opposed to #host_with_port), and the fullpath
(pathname + query params)

Another benefit of this changeset is that it no longer mutates the
original env object (assigning to `new_env` is by reference).

Caveat: im dropping @options[:protocol] bc there's x-fwd-proto should
handle this already.
@Peleg
Copy link
Author

Peleg commented Apr 6, 2020

whops. i meant to open this pr against my own fork. since this is an issue that might affect others, im gonna leave it open here for discussion. let me know if you have any questions or if you want options[:protocol] back for backward compat (EDIT: options[:protocol] was reinstated in
22467c9)

@kshsieh kshsieh force-pushed the pr-fix-host branch 2 times, most recently from 26a8545 to 27b1ea5 Compare May 13, 2020 20:16
- Reinstating custom support for CF-VISITOR CloudFlare header +
  options[:protocol]

- Fixing failing tests
  Instead of passing in `X-Forwarded-Proto` to the Rack env, we can now
  safely rely on `HTTP_` env variables which are automatically set. Per
  https://github.com/rack/rack/blob/master/SPEC.rdoc#the-environment-:

      HTTP_ Variables:
      Variables corresponding to the client-supplied HTTP request headers
      (i.e., variables whose names begin with HTTP_). The presence or absence
      of these variables should correspond with the presence or absence of the
      appropriate HTTP header in the request. See RFC3875 section 4.1.18 for
      specific behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant